Skip to content

test: cover runtime benchmark script#304

Merged
ndycode merged 3 commits intomainfrom
test/pr10-benchmark-runtime-path-smoke
Mar 23, 2026
Merged

test: cover runtime benchmark script#304
ndycode merged 3 commits intomainfrom
test/pr10-benchmark-runtime-path-smoke

Conversation

@ndycode
Copy link
Owner

@ndycode ndycode commented Mar 22, 2026

Summary

  • add a focused smoke test for scripts/benchmark-runtime-path.mjs
  • verify the script writes a JSON benchmark payload with the expected result entries
  • improve coverage for a previously untested dist-backed entrypoint script

Validation

  • npm run typecheck
  • npm run lint -- test/benchmark-runtime-path-script.test.ts
  • npm run test -- test/benchmark-runtime-path-script.test.ts
  • manual QA: build and run node scripts/benchmark-runtime-path.mjs --iterations=1 --output=<tmp>/runtime-benchmark.json

note: greptile review for oc-chatgpt-multi-auth. cite files like lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.

Greptile Summary

adds a focused smoke test for scripts/benchmark-runtime-path.mjs that spawns the script with stub dist fixtures and verifies the written json payload contains the expected result entry names. both prior blocking concerns (bare rmSync in cleanup, unused dirname import) are fully addressed in this revision.

  • afterEach correctly uses removeWithRetry with { recursive: true, force: true } — windows filesystem safety is satisfied
  • import block is clean: only join from node:path, no dead imports
  • fixture construction properly creates the stub dist/lib/... files so the script's static esm imports resolve against the temp dir, not the real dist/
  • the one remaining gap is that spawnSync has no timeout option (hang risk) and doesn't surface result.stderr in assertion failures, making CI debugging harder if the child process crashes

Confidence Score: 4/5

  • safe to merge after adding a spawnSync timeout and stderr assertion; no correctness or windows safety regressions
  • both prior blocking issues are resolved; the only remaining feedback is a P2 style suggestion (timeout + stderr surfacing on spawnSync). the test logic, fixture setup, and cleanup are correct.
  • test/benchmark-runtime-path-script.test.ts — spawnSync timeout and stderr surfacing

Important Files Changed

Filename Overview
test/benchmark-runtime-path-script.test.ts new smoke test for the benchmark script; prior blocking issues (bare rmSync, unused dirname import) are fully resolved; one remaining P2 — spawnSync lacks a timeout guard and doesn't surface stderr on failure

Sequence Diagram

sequenceDiagram
    participant T as Test (vitest)
    participant F as createRuntimeBenchmarkFixture()
    participant FS as tmp filesystem
    participant S as spawnSync (node)
    participant B as benchmark-runtime-path.mjs

    T->>F: call fixture helper
    F->>FS: mkdtempSync → fixtureRoot
    F->>FS: write stub dist/lib/request/request-transformer.js
    F->>FS: write stub dist/lib/request/helpers/tool-utils.js
    F->>FS: write stub dist/lib/accounts.js
    F->>FS: copyFileSync real script → fixtureRoot/scripts/
    F-->>T: { fixtureRoot, scriptCopy }

    T->>S: spawnSync(node, [scriptCopy, --iterations=1, --output=...])
    S->>B: executes script
    B->>FS: import stubs from ../dist/lib/...
    B->>FS: writeFile(outputPath, JSON payload)
    B-->>S: stdout "Runtime benchmark written: ..."
    S-->>T: { status, stdout }

    T->>FS: readFileSync(outputPath)
    T->>T: assert payload.iterations === 1 && result names match

    T->>T: afterEach → removeWithRetry(fixtureRoot)
Loading

Fix All in Codex

Prompt To Fix All With AI
This is a comment left during a code review.
Path: test/benchmark-runtime-path-script.test.ts
Line: 74-78

Comment:
**`spawnSync` missing `timeout` and stderr surfacing**

no `timeout` option means the test can hang indefinitely if the script gets stuck (e.g., the async promise never resolves). add a reasonable cap. also, when `result.status !== 0`, the test failure message will only show "expected 1 to be 0" — you lose the actual error output from the script, which makes CI failures hard to diagnose. consider asserting stderr is empty or including it in a custom message:

```suggestion
		const result = spawnSync(
			process.execPath,
			[scriptCopy, "--iterations=1", `--output=${outputPath}`],
			{ encoding: "utf8", timeout: 15_000 },
		);

		expect(result.stderr).toBe("");
```

the `expect(result.stderr).toBe("")` line gives you the full error text in the failure diff if the script crashes.

How can I resolve this? If you propose a fix, please make it concise.

Last reviewed commit: "Drop unused benchmar..."

@chatgpt-codex-connector
Copy link

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 22, 2026

Important

Review skipped

Auto reviews are limited based on label configuration.

🚫 Review skipped — only excluded labels are configured. (1)
  • skip-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: a3579c4a-5e70-4428-af6c-866a88820f92

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch test/pr10-benchmark-runtime-path-smoke
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch test/pr10-benchmark-runtime-path-smoke

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ndycode ndycode added the passed label Mar 22, 2026
@ndycode ndycode merged commit b1cee6f into main Mar 23, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant